-
Notifications
You must be signed in to change notification settings - Fork 229
Implement IgnoreExtraFieldsFromTextual
#303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement IgnoreExtraFieldsFromTextual
#303
Conversation
|
@rockwotj Re-implemented the solution. Please review |
|
@rockwotj @mihaitodor any chance you could look at this PR shortly? |
rockwotj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM, some nitpicks during review
| t.Fatalf("expected empty remaining buffer, got: %v", remaining) | ||
| } | ||
|
|
||
| m, ok := native.(map[string]interface{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer any in new code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the support for GO 1.12, we still need to keep interface{}. Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably bump that, it's EOL
json_utils.go
Outdated
| if i >= len(buf) { | ||
| return nil, io.ErrShortBuffer | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check isn't required right? the loop conditional should catch it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
| } | ||
|
|
||
| // skipJSONString skips a JSON string starting with '"' and returns the buffer after the closing '"'. | ||
| func skipJSONString(buf []byte) ([]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's OK, but this doesn't validate the string
|
Merge conflicts @passuied |
|
Addressed your feedback @rockwotj please re-review |
Description
Add ability to ignore extra fields from textual representation
Approach:
Usage example:
Issue Reference
#294